smartcontract: enforce Permission flags on multicast subscribe and create-user handlers#3966
Conversation
juan-malbeclabs
left a comment
There was a problem hiding this comment.
The authorization logic itself is solid: the migration is faithful to the authorize() pattern, the error-variant preservation (NotAllowed vs Unauthorized) in UpdateMulticastGroupRoles is careful, and split_trailing_permission correctly disambiguates all four possible account layouts. The tests prove the cross-owner property and preserve the #3206 contracts.
The blocker is integration, not logic. The branch is based on a merge-base that predates EdgeSeat (#3955 / 0e488f63, already on main) — I confirmed the branch does not contain those commits. Two of the migrated handlers parse their trailing accounts positionally, and that collides with the EdgeSeat metro-gate trailing accounts once rebased. The merge-base diff hides this because they are additions in adjacent regions that git merges textually while producing incorrect semantics.
Each point is detailed inline with a proposed fix. Summary:
- 🔴 High — trailing-account collision with EdgeSeat in
create_subscribe.rsandsubscribe.rsafter rebase. - 🟡 Medium —
NON_MIGRATED_SUBSYSTEMSinsmartcontract/cli/src/permission/audit.rsis now stale (CLAUDE.md / Permissions rule). The file is not in the diff, so I commented oncreate_core.rs. - 🟢 Nit — missing coverage for the n=4 branch of
split_trailing_permission(user_payer + permission together).
Recommendation: rebase onto main and re-verify end-to-end before merging; port the two handlers to the PDA-match approach already used in add.rs. The green suite reported in the PR was necessarily run against the stale base and does not cover this interaction.
…eate-user handlers Continues the Permission-account authorization migration to the remaining multicast/user handlers the feed oracle calls, so the oracle can operate on an ACCESS_PASS_ADMIN | USER_ADMIN Permission account instead of foundation_allowlist membership (least privilege for a hot, always-online key). - UpdateMulticastGroupRoles: granting roles on another owner's pass is now an ACCESS_PASS_ADMIN operation; removal-only cleanup stays USER_ADMIN. The historical NotAllowed/Unauthorized error variants are preserved. - CreateSubscribeUser owner-override now accepts a USER_ADMIN Permission holder in addition to the sentinel and foundation, threaded through create_user_core. - Both handlers disambiguate the optional trailing Permission account from the EdgeSeat feed/device accounts via split_trailing_permission (PDA match, not position), so Permission and Feed coexist unambiguously. This also fixes a latent ordering bug in create_subscribe's EdgeSeat feed wiring: the SDK's assemble_instructions always appends [payer, system, permission] last, so the feed sits before payer/system, not after (feed_metro_gate_test updated to the SDK-consistent order). - permission/audit.rs: NON_MIGRATED_SUBSYSTEMS text updated -- the user-create owner override now honors USER_ADMIN via authorize(), leaving only the sentinel direct check un-migrated. - Adds ACCESS_PASS_ADMIN coverage for the subscriber-allowlist path (the processors themselves were migrated upstream in #3982). - PERMISSION.md: document domain-instruction enforcement for the multicast / user surface, and correct legacy-mapping rows that were stale after the activator retirement. The Permission path carries no owns-it restriction, so a flag holder acts across owners, which the oracle needs for validator-owned users and passes. The change is additive: every existing caller keeps its current authority. Refs: RFC-2 (malbeclabs/infra#1764), malbeclabs/doublezero-shreds#517
b62f49d to
8061a23
Compare
There was a problem hiding this comment.
All review points addressed in 247ea4a and verified in the tree:
- 🔴 create_subscribe.rs and subscribe.rs: ported to
split_trailing_permission; the Permission is disambiguated by PDA and Feed/device coexist without collision. - 🟡 audit.rs:
NON_MIGRATED_SUBSYSTEMSupdated — the owner override now honors USER_ADMIN via authorize(), leaving only the direct sentinel_authority check un-migrated. - 🟢 add.rs: took main's version (#3982) during the rebase.
- 🟢 n=4 coverage: accepted via the helper's unit tests.
LGTM.
Summary of Changes
ACCESS_PASS_ADMIN | USER_ADMINPermission account instead offoundation_allowlistmembership — least privilege for a hot, always-online key.UpdateMulticastGroupRoles: granting roles on another owner's pass is now anACCESS_PASS_ADMINoperation, while removal-only cleanup staysUSER_ADMIN(unchanged from [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions #3206). Splitting by operation preserves the deliberate "USER_ADMIN is removal-only here" contract and the exact historicalNotAllowed/Unauthorizederror variants.CreateSubscribeUserowner-override now accepts aUSER_ADMINPermission holder in addition to the sentinel and foundation. The Permission account is threaded throughCreateUserCoreAccounts(Nonefor plainCreateUser, which never overrides the owner).split_trailing_permission(PDA match, not position), so Permission and Feed coexist unambiguously. This also fixes a latent ordering bug increate_subscribe's EdgeSeat feed wiring: the SDK'sassemble_instructionsalways appends[payer, system, permission?]last, so the feed sits before payer/system, not after — only latent today becausefeed_pkisNoneeverywhere.permission/audit.rs: corrects theNON_MIGRATED_SUBSYSTEMStext — the user-create owner override now honorsUSER_ADMINviaauthorize()(foundation via the legacy mapping, Permission holders directly), leaving only the sentinel direct check un-migrated.Context: RFC-2 (malbeclabs/infra#1764), malbeclabs/doublezero-shreds#517. The subscriber-allowlist handlers (
Add/Remove MulticastGroupSubAllowlist) were migrated toauthorize()upstream in #3982 while this branch was in review, so that half is dropped here; this PR completes the set for the subscribe/create-user surface, following the establishedauthorize()+split_trailing_permissionpattern with no new authorization helper.Diff Breakdown
Overwhelmingly tests: the authorization logic itself is a small, additive change to three processors, with the bulk of the diff proving the new cross-owner Permission paths.
Key files (click to expand)
smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs—UpdateMulticastGroupRolesgrants viaACCESS_PASS_ADMIN/ cleanup viaUSER_ADMIN; peels the trailing Permission by PDA match, deriving device/feed from the leading listsmartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs— owner-override authorizes aUSER_ADMINPermission holder alongside the sentinelsmartcontract/programs/doublezero-serviceability/src/processors/user/create_subscribe.rs— parses[feed?, payer, system, permission?]viasplit_trailing_permission, threading both feed and Permission intocreate_user_coresmartcontract/cli/src/permission/audit.rs— updatesNON_MIGRATED_SUBSYSTEMSsodoublezero permission auditno longer over-reports the owner overridesmartcontract/programs/doublezero-serviceability/src/processors/user/create.rs— passespermission_account: None(plainCreateUsernever overrides the owner)Testing Verification
test_subscribe_access_pass_admin_permission_allowed— anACCESS_PASS_ADMINholder (not owner/foundation/sentinel/feed) subscribes a user cross-owner.test_create_subscribe_user_user_admin_owner_override— aUSER_ADMINholder creates a subscribe-user withowner != payer.test_subscribe_user_admin_permission_rejected(USER_ADMIN still cannot add roles) andtest_unsubscribe_user_admin_permission_allowed(USER_ADMIN removal cleanup).test_multicast_subscriber_allowlist_access_pass_admin_permission) — the oracle's exact path, which serviceability: authorize multicast instructions via Permission accounts #3982 migrated without adding a test.feed_metro_gate_testto the SDK-consistent account order (feed before payer/system); all 4 EdgeSeat metro-gate cases pass, confirming feed + the new Permission parsing coexist.test_set_accesspass_refills_depleted_user_payeris a pre-existing failure onmain(airdrop-refill balance assertion, unrelated to authorization) — confirmed it fails identically on a cleanorigin/maincheckout.